-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[firebase_remote_config] Remote config/double instance error #2061
[firebase_remote_config] Remote config/double instance error #2061
Conversation
Is the failing check known to behave flakily? 🤔 |
Also, I'm open to alternative solutions to fix the error "Future already completed", I just wanted to fix the tests Interestingly, this does work and gives back a |
RemoteConfigSettings get remoteConfigSettings => _remoteConfigSettings; | ||
|
||
static Completer<RemoteConfig> _instanceCompleter = Completer<RemoteConfig>(); | ||
@visibleForTesting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're generally trying to avoid @VisibleForTesting APIs since they clutter the public logs, though I don't see another solution at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree, I tried to find an alternative first before using it, but have to change it back somehow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FirebaseExtended/invertase @collinjackson is there any other solution to be able to test for this edge case?
…ror' into remote_config/double-instance-error
@collinjackson @kroikie Any updates on this? |
@FirebaseExtended/invertase @Ehesp It would be cool to have this fix landed. I've heard a couple of times in our company alone, that people spent quite some time getting to fix this issue, just because it sends a false-positive. This adds a missing test case, where Remote Config is initialized twice in parallel, which leads to an error. The current status of this PR is that @collinjackson wasn't too happy about using I'll be happy to update the CHANGELOG.md if you are happy with the current implementation of the tests. |
Closing in favor of #4186 which makes this PR obsolete! |
Description
This pull request addresses an issue with a test where two instances were created shortly after another leading to a
Future already completed
StateError
.This was caused due to side-effects of the tests where all tests passed when they were executed at once and the
doubleInstance
test failed when executed alone.Also in this Pull Request is a fix for the
doubleInstance
error. Realized with a try/catch for only this exact error. I've used this fix because to me it feels much more elegant compared to a fix where I would basically would have to check for the same shortly after.Alternative solution:
Related Issues
#195
flutter/flutter#38060
#28
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?